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

cpu, cc2538: adapt periph uart to GPIO API #7310

Merged
merged 4 commits into from
Dec 14, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Jul 4, 2017

this PR adapts low-level UART driver of TI CC2538 to use RIOTs GPIO API.

Its part of adaption to solve #6650, next steps are to rework I2C and SPI, too.

@smlng smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 4, 2017
@smlng smlng self-assigned this Jul 4, 2017
@smlng smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 4, 2017
@smlng
Copy link
Member Author

smlng commented Jul 4, 2017

should be rebase as soon as #7303 is merged

@smlng smlng added this to the Release 2017.10 milestone Jul 4, 2017
@smlng
Copy link
Member Author

smlng commented Jul 4, 2017

rebased

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 4, 2017
@smlng smlng force-pushed the cpu/cc2538/periph_uart branch 5 times, most recently from b63a964 to 65876b6 Compare July 4, 2017 13:56
@smlng smlng added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 5, 2017
@smlng
Copy link
Member Author

smlng commented Jul 5, 2017

rebased onto #7316

@smlng smlng added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 5, 2017
@smlng smlng requested a review from a user July 5, 2017 07:14
@smlng smlng mentioned this pull request Jul 5, 2017
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Everything looks good. I have found nothing.

@smlng smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 7, 2017
@smlng smlng force-pushed the cpu/cc2538/periph_uart branch 2 times, most recently from 2964321 to 9c22b13 Compare September 7, 2017 13:35
@danpetry danpetry mentioned this pull request Nov 22, 2017
7 tasks
@smlng
Copy link
Member Author

smlng commented Nov 28, 2017

rebased

@@ -26,29 +26,12 @@
#include "cc2538_gpio.h"
#include "periph_cpu.h"
#include "periph_common.h"
#include "periph/gpio.h"
Copy link
Member

Choose a reason for hiding this comment

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

Needed?

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 don't think so, might be an artefact from rebasing

Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

Minor things to clarify. Besides I've succesfully tested on remote-reva/b.

#define UART_0_TX_PIN GPIO_PIN(0, 1) /**< GPIO_PA1 */
#define UART_0_RX_PIN GPIO_PIN(0, 0) /**< GPIO_PA0 */
#define UART_0_RTS_PIN GPIO_PIN(3, 3) /**< GPIO_PD3 */
#define UART_0_CTS_PIN GPIO_PIN(1, 0) /**< GPIO_PB0 */

/* UART 1 device configuration */
Copy link
Member

Choose a reason for hiding this comment

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

Any ideas why this half-done configuration is 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.

Nope no idea, I mean the cc2538 has 2 UARTS, but I'm not sure if both are accessible on the DK version - but should be, I'll check and maybe add the correct pin config - with RTS CTS then.

#define UART_0_CTS_PIN GPIO_PB0
#define UART_0_TX_PIN GPIO_PIN(0, 1) /**< GPIO_PA1 */
#define UART_0_RX_PIN GPIO_PIN(0, 0) /**< GPIO_PA0 */
#define UART_0_RTS_PIN GPIO_PIN(3, 3) /**< GPIO_PD3 */
Copy link
Member

Choose a reason for hiding this comment

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

In the driver implementation below it is written /* On the CC2538, hardware flow control is supported only on UART1 */ and that is also what the code represents. UART_0 however, maps to UART0 here. I'm sorry, obviously this was not intendet to fix with this PR, but could you clarify this please?

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 admit I just adapted to use GPIO_PIN without questioning the general config, nor consulting the data sheet on that matter. So you mean RTS and CTS pins should be removed, right?

Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

Whoops, accidentally hit approve before

@danpetry
Copy link
Contributor

danpetry commented Dec 13, 2017

Hi guys, I was just wondering if there's any movement on this? I'm changing cpu/cc2538/periph/uart.c for issue #7941 and would be good to avoid merge conflicts. I will probably be submitting a PR for this in the next few days. I could work around your changes i.e. leave the functionality you moved to the GPIO driver alone, but would be neater to merge this first if possible I think

@smlng
Copy link
Member Author

smlng commented Dec 13, 2017

I'll rebase to resolve conflicts, otherwise this needs approval to be merged.

@danpetry
Copy link
Contributor

@PeterKietzmann was wondering if there would be any chance of doing this sometime in the near future? :)

@smlng
Copy link
Member Author

smlng commented Dec 14, 2017

rebased

@PeterKietzmann
Copy link
Member

Will have an other look this afternoon

@danpetry
Copy link
Contributor

Thx guys

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 14, 2017
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

I've tested again (on a remote-revb this time) and STDIO works as expected. ACK for this change, even though my concerns about the hardware flow control configuration have not been addressed. But this is an other issue.

@PeterKietzmann PeterKietzmann merged commit ec2d7c6 into RIOT-OS:master Dec 14, 2017
@smlng smlng deleted the cpu/cc2538/periph_uart branch February 6, 2018 19:42
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants