Skip to content

cpu/lpc1768: add gpio peripheral (+ board adaptions)#6472

Merged
basilfx merged 3 commits intoRIOT-OS:masterfrom
basilfx:feature/lpc1768_gpio
Apr 3, 2018
Merged

cpu/lpc1768: add gpio peripheral (+ board adaptions)#6472
basilfx merged 3 commits intoRIOT-OS:masterfrom
basilfx:feature/lpc1768_gpio

Conversation

@basilfx
Copy link
Member

@basilfx basilfx commented Jan 24, 2017

I needed GPIO on the LPC1768 (more specific, on the Seeeduino Arch Pro), but I realized it wasn't there. Since I need it for a demo tomorrow, I decided to add it :-)

So far output mode works, but that's all I tested. Have to dig into interrupts, which seem to be a bit more complex.

Adapted mbed_lpc1768 and seeeduino-arch_pro boards, including SAUL support for GPIO. I realize that they overlap a lot, but the boards are not quite similar.

WIP, needs a lot of cleanup.

@PeterKietzmann PeterKietzmann added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jan 25, 2017
@aabadie
Copy link
Contributor

aabadie commented Mar 1, 2017

is this still works in progress @basilfx ?

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.

ping @basilfx

I tested on a mbed_lpc1768 board : the leds are inverted.

* - LED2: P1.20
* - LED3: P1.21
* - LED4: P1.23
* @brief Initialize the boards on-board LEDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

"boards on-board" => on-board is enough

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

#define LED3_ON (LED_PORT->FIOCLR = LED3_MASK)
#define LED3_OFF (LED_PORT->FIOSET = LED3_MASK)
#define LED3_TOGGLE (LED_PORT->FIOPIN ^= LED3_MASK)
#define LED0_ON gpio_clear(LED0_PIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keeping the previous defines ?

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 prefer to keep implementation in one place (gpio.c), and not having two different ways to toggle a LED.

{
LPC_GPIO_TypeDef *base = _base(pin);

if (base->FIOSET & _pin(pin)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be done in one line using

base->FIOPIN ^= (1 << _pin(pin));

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

* - LED4: P1.23
*
* The LEDs are active-low (current-sink).
* @brief Initialize the boards on-board LEDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment about on-board leds

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

#define LED3_ON (LED_PORT->FIOSET = LED3_MASK)
#define LED3_OFF (LED_PORT->FIOCLR = LED3_MASK)
#define LED3_TOGGLE (LED_PORT->FIOPIN ^= LED3_MASK)
#define LED0_ON gpio_clear(LED0_PIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is inverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

On the Seeeduino Arch-Pro, they are inverted :-)

@aabadie
Copy link
Contributor

aabadie commented Apr 5, 2017

ping @basilfx

@basilfx
Copy link
Member Author

basilfx commented Apr 5, 2017

@aabadie Sorry, I'm very busy with work-work. Will have to wait.

@aabadie
Copy link
Contributor

aabadie commented Jun 20, 2017

ping @basilfx. Any chance to apply the requested changes for the next release ?

@aabadie
Copy link
Contributor

aabadie commented Jan 5, 2018

@basilfx any chance for this one to be ready for the release ?

@smlng smlng removed this from the Release 2018.01 milestone Jan 12, 2018
@MrKevinWeiss MrKevinWeiss mentioned this pull request Mar 23, 2018
7 tasks
@basilfx basilfx force-pushed the feature/lpc1768_gpio branch from af263b6 to d8f7a1d Compare March 30, 2018 22:18
@basilfx basilfx removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Mar 30, 2018
@basilfx
Copy link
Member Author

basilfx commented Mar 30, 2018

@aabadie I finally found the time to fix this one.

Re-implemented the GPIO driver, added support for interrupts and fixed board LEDs.

@basilfx basilfx added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 30, 2018
@basilfx basilfx changed the title cpu/lpc1768: add gpio peripheral (+ board adaptions) (WIP) cpu/lpc1768: add gpio peripheral (+ board adaptions) Mar 31, 2018
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.

Looks good codewise, just a minor thing.

Otherwise, I tested it and it works now. Well done !

DEBUG_ADAPTER ?= dap

# this board uses openocd
export IMAGE_FILE = $(HEXFILE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks unrelated. Maybe #8845 will help ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's needed to flash the HEX file instead of the ELF file. #8845 would help, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to make this change in a separate PR. Is it ok for you ?

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'll put this in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #8866.

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.

#8866 is merged, please rebase and squash.

ACK

@basilfx basilfx force-pushed the feature/lpc1768_gpio branch from a50c6e3 to 18e6e5c Compare April 3, 2018 11:31
@basilfx
Copy link
Member Author

basilfx commented Apr 3, 2018

Rebased and squashed.

@basilfx basilfx removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 3, 2018
@basilfx basilfx force-pushed the feature/lpc1768_gpio branch from 18e6e5c to ceb8487 Compare April 3, 2018 14:13
@basilfx basilfx force-pushed the feature/lpc1768_gpio branch from ceb8487 to 29b3798 Compare April 3, 2018 14:48
@basilfx
Copy link
Member Author

basilfx commented Apr 3, 2018

Murdock is happy!

@basilfx basilfx merged commit 77ad70e into RIOT-OS:master Apr 3, 2018
@basilfx basilfx deleted the feature/lpc1768_gpio branch January 14, 2020 22:02
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 Platform: ARM Platform: This PR/issue effects ARM-based platforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants